Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ts_package extension #12

Merged
merged 1 commit into from
Apr 27, 2021
Merged

Conversation

higebu
Copy link
Contributor

@higebu higebu commented Apr 20, 2021

This PR adds the grpc.gateway.protoc_gen_grpc_gateway_ts.options.ts_package option to specify the package name of proto files.

For example, we can add following lines to google/protobuf/empty.proto with package name "google-protobuf/google/protobuf/empty_pb".
Then we can use @types/google-protobuf package for google.protobuf.Empty.

import "protoc-gen-grpc-gateway-ts/options/ts_package.proto";
option (grpc.gateway.protoc_gen_grpc_gateway_ts.options.ts_package) = "package_name";

Related to: protocolbuffers/protobuf-javascript#47

Copy link
Collaborator

@lyonlai lyonlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, was very busy last week. So this is more like an alias proto then we could point the import from an existing npm package of other generated code.

This is cool, thanks for your contribution.

Also a question to you, I prefer to see a test for this feature. Is there any good generated package you know that we could import for a test purpose?

if os.IsNotExist(err) {
return false, nil
if pkg, ok := r.TSPackages[target]; ok {
log.Debugf("pkg: %s", pkg)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have a better debug message? saying package import override %s has been found for file%s

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your review. I will fix this ASAP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyonlai I fixed the debug message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think google-protobuf package is good for testing. I try to write a test. Please wait for a while.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyonlai I added integration test for ts_package option.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice.

@@ -25,5 +26,8 @@
"mocha": "^8.2.0",
"ramda": "^0.27.1",
"typescript": "^4.0.5"
},
"dependencies": {
"google-protobuf": "^3.15.8"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this also be a devDependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I fixed this.

@lyonlai
Copy link
Collaborator

lyonlai commented Apr 27, 2021

Thank you very much for you contribution.

@lyonlai
Copy link
Collaborator

lyonlai commented Apr 27, 2021

would you mind to squash the commit into 1 before merge? @higebu

@higebu
Copy link
Contributor Author

higebu commented Apr 27, 2021

@lyonlai Done!

@lyonlai
Copy link
Collaborator

lyonlai commented Apr 27, 2021

thank you, will merge once test finished green

@lyonlai lyonlai merged commit 4dae0ad into grpc-ecosystem:master Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants